Fix handling of enum default values in RDF and RDG #63086
Conversation
github-actions
bot
added
the
area-networking
There was a problem hiding this comment.
Pull Request Overview
This PR fixes handling of enum default values in Request Delegate Factory (RDF) and Request Delegate Generator (RDG) to ensure proper type casting and formatting. The changes address an issue where enum default values were not being handled correctly in code generation scenarios.
Key changes:
- Enhanced default value string generation to properly handle enum types with correct casting syntax
- Added proper type conversion for enum default values in expression trees
- Added comprehensive test coverage for enum default values including nullable enums
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Shared/RoslynUtils/SymbolExtensions.cs | Added enum-specific handling in default value string generation with proper casting syntax |
| src/Http/Http.Extensions/src/RequestDelegateFactory.cs | Added CreateDefaultValueExpression method to handle enum type conversions in expression trees |
| src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.SpecialTypes.cs | Added test cases for enum default values and modernized array syntax |
Comments suppressed due to low confidence (1)
src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.SpecialTypes.cs:212
- The test case shows nullable enum with non-null value marked as
falsefor the last parameter, but there's no test case for nullable enum with null value marked astrue. Consider adding a test case like["TodoStatus?", "null", null, true]to ensure null handling is properly tested.
["TodoStatus?", "TodoStatus.Done", (TodoStatus?)TodoStatus.Done, false],
captainsafia
force-pushed
the
safia/fix-rdg-enum-default
branch
from
f1169d2 to
672b04b
Compare
|
@BrennanConroy Can I trouble you for a review? This helps unblock dotnet-monitor's migration to native AOT. |
| // Convert(bodyValue ?? SomeDefault, Todo) | ||
| return Expression.Convert( | ||
| Expression.Coalesce(BodyValueExpr, Expression.Constant(parameter.DefaultValue)), | ||
| Expression.Coalesce(BodyValueExpr, CreateDefaultValueExpression(parameter.DefaultValue, typeof(object))), |
There was a problem hiding this comment.
BodyValueExpr is typed object (ref) so we need the types to match here.
| ["ushort", "ushort.MinValue", ushort.MinValue, true], | ||
| ["TodoStatus", "TodoStatus.Done", TodoStatus.Done, true], | ||
| ["TodoStatus", "TodoStatus.InProgress", TodoStatus.InProgress, true], | ||
| ["TodoStatus", "TodoStatus.NotDone", TodoStatus.NotDone, true], |
There was a problem hiding this comment.
| ["TodoStatus", "TodoStatus.NotDone", TodoStatus.NotDone, true], | |
| ["TodoStatus", "TodoStatus.NotDone", TodoStatus.NotDone, true], | |
| ["TodoStatus", "1", 1, true], |
Enum types don't match
There was a problem hiding this comment.
The compiler technically doesn't allow this syntax without an explicit conversion via (TodoStatus)1. I believe you can only do TodoStatus someEnum = 1 if you emit the IL yourself. We can test with the explicit cast though.
| ["decimal", "decimal.One", decimal.One, true], | ||
| ["decimal", "decimal.Zero", decimal.Zero, true], | ||
| ["long", "long.MaxValue", long.MaxValue, true], | ||
| ["long", "long.MinValue", long.MinValue, true], |
There was a problem hiding this comment.
| ["long", "long.MinValue", long.MinValue, true], | |
| ["long", "long.MinValue", long.MinValue, true], | |
| ["long", "3.14", 3.14, true], |
Non-enum types don't match
There was a problem hiding this comment.
Ditto the about about casting.
| } | ||
| catch | ||
| { | ||
| converted = targetType.IsValueType ? Activator.CreateInstance(targetType) : null; |
There was a problem hiding this comment.
Actually, I think we can get rid of this case. The Convert.ChangeType can throw if the conversion is not supported (which I think our IsAssignableFrom covers) or if the conversion overflows (which I don't think can functionally happen if the compiler catches you doing something like short foo = 523525242.
Closes #62910.